Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Preprocessors may return promises, but not resolve to 'content'. #3383

Closed

Conversation

seebees
Copy link

@seebees seebees commented Oct 17, 2019

The original API for Preprocessors assumed callback.
Now promises are a valid return.
If an implementer returns a promise from process
but only resolves content via the callback,
then while the result of process is a promise,
it will not resolve to the intended content.
An easy example is for process to be an async function.
e.g.

async function process(content, file, done) {
   const newContent = await magic(content)
   done(newContent)
}

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@AppVeyorBot
Copy link

Build karma 2426 completed (commit 9d6e4cf686 by @seebees)

@karmarunnerbot
Copy link
Member

Build karma 27 completed (commit 9d6e4cf686 by @seebees)

@karmarunnerbot
Copy link
Member

Build karma 28 completed (commit 9d6e4cf686 by @seebees)

The original API for Preprocessors assumed callback.
Now promises are a valid return.
If an implementer returns a promise from `process`
but only resolves content via the callback,
then while the result of `process` is a promise,
it will not resolve to the intended content.
An easy example is for `process` to be an `async` function.
e.g.
```
async function process(content, file, done) {
   const newContent = await magic(content)
   done(newContent)
}
```
@AppVeyorBot
Copy link

Build karma 2428 completed (commit cc81357af3 by @seebees)

@karmarunnerbot
Copy link
Member

Build karma 30 completed (commit cc81357af3 by @seebees)

@karmarunnerbot
Copy link
Member

Build karma 29 completed (commit cc81357af3 by @seebees)

@seebees
Copy link
Author

seebees commented Oct 18, 2019

A link to the "breaking" PR: #3376
for historical purposes.

@johnjbarton johnjbarton changed the title fix: Preprocessors may return promises, but not content. fix: Preprocessors may return promises, but not resolve to 'content'. Oct 18, 2019
Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for finding this issue. The author of the async change has a similar fix in PR #3387 with unit tests for both paths.

@seebees
Copy link
Author

seebees commented Oct 18, 2019

even better! I would add a comment since this is a little edgy, but that is a style choice :)
Do you mind if I ask what the ETA for release is?

@seebees seebees closed this Oct 18, 2019
@johnjbarton
Copy link
Contributor

4.4.1 is up

@seebees seebees deleted the preprocessors_and_promises branch October 21, 2019 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants